Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new cmake harness, round 3 #233

Open
wants to merge 187 commits into
base: features/new-cmake-harness
Choose a base branch
from

Conversation

loriab
Copy link
Collaborator

@loriab loriab commented Jan 13, 2022

At present, this PR is so EFV can oversee proposed changes to #205. It configures, but I haven't tried so much as building the compiler.

Added 19 Jan

  • switched boost target from generic Boost::boost to h-o Boost::headers allowed in CMake 3.15
  • removed some cmake min version boilerplate left over from external_project structure
  • forced add_library(libint-libcompiler STATIC after build failed with BUILD_SHARED_LIBS=ON
  • updated find_project(Python and result vars since CMake Python detection improved much around 3.15
  • change DATADIR so that it's relative path in Config $<INSTALL_INTERFACE:DATADIR="\$\{_IMPORT_PREFIX\}/${CMAKE_INSTALL_DATADIR}">
  • rearrange the six exported libraries into defines, features, properties, includes, link_libraries
  • switches ${PN} and ${NS} abbreviation variables to ${L2} as the former generic names can get overwritten by a call (e.g., find_package() where another project assigns them)
  • return to 3-char orderings in Config (e.g., gss for gaussian-standard-standard) since I think the 2-char ss were in anticipation of runtime-selectable solid harmonics. (less a priority for psi now)

WIP c. 19 Jan

  • bumped CMake to 3.18 for list sorting in int_am.cmake. will revert to 3.16 at the end unless new requirements surface.
  • start upgrade guide and options description in INSTALL.md
  • detect Eigen if needed in Config file
  • FindTargetEigen3
  • int_am finalization (need to show where new cmake harness #205 not behaving. need to handle G12 properly)
  • organize exports to targets files better
  • handle library targets as components in Config better

Added 20 Jan

  • moved configure.ac and config.h.in to a archived_libool/ dir so they can't participate unbeknownst to us
  • have only the 3 definitive version lines (version, buildid, soversion) in top-level CM file. leave derived version vars and handoff to library target to cmake modules. define versions in project() (top-level and library export) so that usual _MAJOR, PROJECT_TWEAK, etc. that ppl may expect are defined
  • install basis/ into versioned location again. should it really be share/libint/version/basis, not libint2?

Added 21 Jan

  • check user can overwrite all the install locations and they get passed from generator correctly and all captured in the final library install to prefix.
  • gave up defaulting CMAKE_INSTALL_LIBDIR to lib. probably more natural to stick with standard GNUInstallDirs. renamed (back) the custom install vars LIBINT2_INSTALL_BASISDIR and LIBINT2_INSTALL_CMAKEDIR so as to not invent new vars starting CMAKE_
  • address pkgconfig paths issue mentioned at 205
  • expand Libint2:: aliases so that available internally. use these for tests and drop the int-library, etc. series.

Added 24 Jan

  • simplify enable_testing and build_testing sequences for new cmake and test them.
  • add onebody, header-only, and pure_am components for psi's benefit. document/copy-over program-specific parameters.
  • make -> cmake for doxygen. it generates files, but I haven't examined them.
  • take adv of newer cmake to skip DESTINATION where can. Make sure each install() for library goes to Libint2_Runtime or Libint2_Development

Added 28 Jan

  • rename FindMPFR.cmake to FindMultiprecision.cmake to handle gmp, gmpxx, and mpfr targets. Based on offical cmake module FindGSL.cmake to hopefully handle Windows, too. Seek correct targets in build_libint and library.
  • remove old-style cmake variables like Libint2_LIBRARIES from config. added back Libint2_EXT_VERSION. Rearrange config so no targets created before "found" assured.
  • make sure CMAKE_PREFIX_PATH passes to ExternalProject even when list by placing in Cache. Only install bundled Boost if CXX activated
  • try to standardize cmake formatting (on lines I've touched for worthier reason) to 4 spaces normal and 2 spaces w/i command.
  • Fortran work. Setting both CMAKE_Fortran_MODULE_DIRECTORY and property Fortran_MODULE_DIRECTORY seemed redundant. Switched to only the latter and added the path to include directories on the target. Looking on the internet, where to install module files looks like it has no guidelines (esp. since compiler-version-dependent) so added an install knob. Added fpic when necessary so tests work with shared lib.

Added 12 Feb

  • renamed GHA workflow to "CI". split it into two jobs, the first of which build_repo, as before, builds and runs the compiler, builds the library, tests the library, tests a library consumer for Lin/Mac Debug/Release, all static libs. To this was added artifacting the built tarball and building on Windows (Release only). The Windows build is an expected fail -- the generated source is faulty. The second and new job build_export takes the linux tarball artifact and builds it on Lin/Mac/Win, shared libs where possible, with some additional dependency quirks testing. Changed build matrix from include format to cfg format, as I think it's easier to read. Switched Debug/Release EP/FetchContent pairs as Windows only works with 1/4 combinations.
  • Reverted minimal CMake to 3.16 with a fn in int_am.cmake . Test exactly 3.16 in GHA.
  • Bump CMake version to 2.7.1 from 2.7.0-beta.6 to match configure.ac
  • Added an option REQUIRE_CXX_API_COMPILED to turn on/off the compiled C++11 interface apart from the header-only library. It's on by default, so no change.
  • Set up ENABLE_T1G12_SUPPORT missing before.
  • Added G12 ints. new macro in int_am.cmake to handle non-list, etc. and components
  • Same for G12DKH but these throw error at compiler exe or lib build stage.
  • renamed int-cxx-{static,shared} to int-cxx-compiled-{static,shared} for clarity
  • reworked Eigen3 detection. copied a FindEigen3.cmake from latest Eigen release. This is installed with L2 and the local path is prepended so this FindEigen3 is always used. The FindEigen already handles (1) finding installations with a Eigen3Config.cmake, (2) seeking if the Eigen3_INCLUDE_DIR already in cache, (3) searching for header like a traditional FindPkg.cmake. It always returns a target, Eigen3::Eigen. This has been modified to address some of your concerns. (1) turned off build package registry to avoid the ephemeral build trees. (2) If an option LIBINT_LOCAL_Eigen3_FIND is set, the FindEigen instead looks to import a local L2-defined target (see below) to ensure ABI consistency. This is off by default. The FindEigen3 detection is shared by the library build and libint2-config.cmake
  • Eigen3 handling on the src/lib/libint/CM.export side is outsourced to FindEigen3. If LIBINT_LOCAL_Eigen3_INSTALL is set, the detected eigen3 is installed as a target in a separate file. This is off by default, but I don't care if it's on by default, so long as it's turn-off-able and the _FIND option is off by default.
  • fixed some locale problems for Windows in basis.h. Added some ifdefs for gnu compilers that I hit when using the wrong Windows compiler. Left them, but not really needed.
  • removed a seemingly redundant basisset file. no trouble so far.
  • Fixed Mac shared libraries that weren't linking
  • Half fixed Win shared libraries that aren't exposing fn symbols (fixed with windows_export_all_symbols) or libint2_build_* data symbols (not fixed; see issue)
  • half install target for fortran interface. (target installed, export written but not installed.) I don't know if it's wanted or generally redistributable enough.
  • consolidated the main install(target and install(export according to my latest reading of best cmake practices.

WIP

  • Still working on INSTALL.md and general tidying
  • Do you want ENABLE_XHOST to autoselect optimization for current instruction set? It's gotten more complicated (2 -> 20 lines)
  • Should I mark the name int2-cxx as provisional? I find the name confusing, and the library isn't testable.
  • The components names C, CXX, CXX_ho, I keep getting wrong, which probably means they're not intuitive.
  • Merge README.md and INSTALL.md?

@loriab loriab mentioned this pull request Jan 14, 2022
16 tasks
@evaleev evaleev force-pushed the features/new-cmake-harness branch 2 times, most recently from f89be3c to b13667b Compare January 14, 2022 20:30
@evaleev
Copy link
Owner

evaleev commented Jan 16, 2022

@loriab CI passes!

how painful would be to revert to cmake 3.16?

@loriab
Copy link
Collaborator Author

loriab commented Jan 17, 2022

@loriab CI passes!

Thanks!

How painful would be to revert to cmake 3.16?

It won't be too bad. I figured I'd keep it at 3.18 for int_am readability for now (2 lines to compute each max instead of ~7) then revert to 3.16 at the end. I have updated some Python and Boost things that could be used only >=3.15.

@loriab
Copy link
Collaborator Author

loriab commented Jan 17, 2022

Sorry to leave CI broken after you so kindly fixed it. This shows a strategy for separating machine-path-dependent targets (that you described the need for but are unsuitable for distribution) from the usual redistributable targets. This also fixes the full paths in the installed target tree for the latter. I'm still working on shifting eigen3 detection, so CI has no chance at the moment.

@evaleev
Copy link
Owner

evaleev commented Jan 17, 2022

@loriab no worries

BTW I am not sure I understand why we need to have the "nonlocal" (path-independent) targets. If you allow the user to discover Eigen3 on their own or provide their own Eigen3 target there is no way to make sure that it is configured correctly (e.g. the one used by Libint to pre-instantiate Engine used Eigen3 configured to need BLAS/LAPACK ... there is no way to test Eigen3's configuration without much try_compile code).

This is a common scenario for C++ and even for C code. E.g. CMake's standard MPI targets are ABI-specific, so it is not possible to say find_package(MPI COMPONENT mpich_abi ... ) or find_package(MPI COMPONENT sgi_abi ... ) ... the only way to avoid ABI problems in general is to hardwire to the particular instance of a target.

Is this a requirement motivated by package maintenance??

@loriab
Copy link
Collaborator Author

loriab commented Jan 18, 2022

BTW I am not sure I understand why we need to have the "nonlocal" (path-independent) targets. If you allow the user to discover Eigen3 on their own or provide their own Eigen3 target there is no way to make sure that it is configured correctly (e.g. the one used by Libint to pre-instantiate Engine used Eigen3 configured to need BLAS/LAPACK ... there is no way to test Eigen3's configuration without much try_compile code).

This is a common scenario for C++ and even for C code. E.g. CMake's standard MPI targets are ABI-specific, so it is not possible to say find_package(MPI COMPONENT mpich_abi ... ) or find_package(MPI COMPONENT sgi_abi ... ) ... the only way to avoid ABI problems in general is to hardwire to the particular instance of a target.

Is this a requirement motivated by package maintenance??

Yes, I'm thinking of packaging and accommodating the up to three computers that build library source, build library, and build library consumer. All I really want in the end is:

  • be able to generate a libint2 library source tarball to build on Lin/Mac/Win
  • agree amongst the packagers (fedora, debian, conda-forge) on a set of libint config options such that a single built library can service all open-source QC programs (built packages) and be checkable through components at library detection time.
  • be able to continue to supply a built libint2 conda package so that psi4 devs and users can build psi4 against it pretty cheaply. Almost all use the conda eigen3 package, of fixed relative location wrt libint, but that's not a constraint.

I think I'm reading code right that the Libint2 cxx interface at present is header only. So the detection at library-build time in src/lib/libint/CMakeLists.txt.export is perfunctory so that the eigen target can be recorded in libint2-config.cmake as a dependency of Libint2::cxx interface library target. The eigen code only gets compiled within the libint repo for the tests that use the cxx interface. If I have all that right, I feel on pretty firm cmake footing that there shouldn't be full paths in the config file and one doesn't generally install external dependency targets (https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages). The latter is unusual enough that there's a new v3.21 capability to allow it (https://cmake.org/cmake/help/latest/command/install.html#installing-runtime-dependencies). I'd expect the eigen detection to happen in a find_dependency(eigen-like) in the libint-config file. I'm working on wrapping the call so it allows plain header location and avoiding the build tree, not just the discoverability of Eigen3Config.cmake . In this libint2-cxx-interface-header-only case, this eigen code detected at built-libint detection time is the only eigen code involved, so no need to match up exact installations.

Your plan of non-header-only Libint2 cxx interface surprised me. I gather that the int-cxx-shared target becomes a tangible library with compiled code, not just an interface library. I accept that the same eigen code and offloading defines ought to be used for both libint-building and libint-consumer-building. This is fairly easy when packagers control both the libint2-building and the e.g., psi4-building steps because they'll use common eigen conditions. I'm not sure how it should work for the more general cmake case, though. Just vendorizing the eigen code into include/libint2/eigen/ doesn't work because it's not just the eigen code you want shared between libint2 and the qc program but also the offloading defines and attendant library linking. Since you seemed to assume that libint2 and QC consumer were build on the same computer anyways by the full paths to eigen, I figured a workaround to preserve normal cmake relocatability for the header-only cxx interface and accommodate the common-computer non-header-only cxx interface you describe would be to separate the targets so packagers can decline to install the latter while users can request the latter.

I hope I'm understanding your points and plans better in these posts. Mostly I've been focusing on the existing header-only state. Thanks for you patience. Pinging @susilehtola in case there's a packaging aspect/constraint that I'm missing, being mostly conda-focused.

@evaleev
Copy link
Owner

evaleev commented Jan 18, 2022

@loriab due to the header-only inefficiencies the plan to make C++ API a proper (compiled) library has been around for awhile (for as long as engine.impl.h existed). There will be no impact on current API users (like Psi), and switching to the compiled version will be trivial: cxx to be a header-only C++API and cxx_{shared,static} a compiled version (for now only including Engine, eventually might include more).

I agree that for careful users the ABI issues will not appear. But I'm just giving you a heads-up for what's likely to happen (and has happened to me multiple times before ... so experts are not immune). Since ABI mismatches are impossible to detect at build-time and in general extremely painful to diagnose the ABI mismatches in my book are more evil than hardwiring to the particular target instance ... perhaps the hardwiring should be the default with an opt-out for experts? OR the other way around? (the latter makes less sense to me: the only reason to hardwire search paths is for an average user, not an expert).

P.S. Eigen is just an fn pain. I'd love to be able to just restrict to config-equipped Eigen but the amount of pushback I get along the lines of "but Eigen is a header-only library!!!! I should not need to install it" is tiresome.

@evaleev
Copy link
Owner

evaleev commented Jan 18, 2022

@loriab btw I made the int-cxx-{static,shared} a compiled library. The header-only (interface) version is int-cxx-headeronly-{static,shared}. Should probably not be used by anyone since cmake is now the only way the C++ interface can be used by mortals

# Conflicts:
#	src/lib/libint/CMakeLists.txt.export
@loriab
Copy link
Collaborator Author

loriab commented Feb 13, 2022

@evaleev, I think this is ready for review. I'm still working on the INSTALL.md docs, but I'm satisfied with the technical situation. If you approve of the general structure and solutions, I'd like to advise packagers that it's ready to test soon, as the problems they find are probably in my court rather than yours. Let me know if you'd like to Zoom to talk over reasoning and concerns in real time for efficiency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants